[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: prateekpandey14
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: childsb

If they are not already assigned, you can assign the PR to them by writing /assign @childsb in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

cc @j-griffith

is it possible to retain the commit history?

Looks like you finished up the work in the old examples/iscsi directory. Haven't tried to load it up and deploy it. There are some deployment details we need to address I think.

Also, we did work up an iSCSI lib/helper here: https://github.com/kubernetes-csi/csi-lib-iscsi
That lib didn't leverage the existing K8s iscsi_utils; in hindsight perhaps it should have. It would be great IMO to:

1. Preserve the commit history for what you brought over as @msau42 mentioned
2. Extract the iscsi_util pieces out of the driver and either use the csi-lib-iscsi or consider using what's in iscsi_util.go for csi-lib-iscsi instead
3. Need to add things like the Dockerfiles and deployment manifests

The only reason for revisiting csi-lib-iscsi is that the code in iscsi_util.go is already tested/used in production so rather than trying to grow a completely new lib maybe we should be leveraging what's already tested/proven.

We'll need to answer the deployment questions including dealing with iscsiadm in containers for this driver though. That means things like the Dockerfiles, Manifests etc.

We could do it as separate prs if that makes things easier to review

@msau42

We could do it as separate prs if that makes things easier to review

Assuming you mean WRT the Dockerfile and deployment work, either way is fine with me, If there are no objections from others. We can also discuss the iscsi-lib portion later as well

Yeah I mean two prs:

1. Purely migrate the existing code over
2. Start making changes on top of it

Thanks @msau42 @j-griffith, I was thinking the same to add the manifest/deploy files in next commit/PR , just wanted to get initial reviews (which I got ☺).

I will work on the review comments.

@msau42 @j-griffith raised PR #2 with retained commit history